Implement pow-by-constant with NAF for FpVar
#72
Conversation
|
The need to support zero bases for a non-zero exp when using NAF has something to do with Poseidon. It is very possible for Poseidon to be powering "0" to alpha. If alpha has a NAF form that contains "-1", it should not make the constraint system unsatisfiable, which needs the |
|
The zero-base zero-pow issue is also discussed in arkworks-rs/algebra#303. If we decide that 0^0 = 1, then this PR can also be simplified somehow (note, only the beginning part). |
| use ark_ff::biginteger::arithmetic::find_wnaf; | ||
|
|
||
| // first check if exp = 0 | ||
| let mut is_nonzero = false; |
There was a problem hiding this comment.
Is this something we can move to a function in utils?
There was a problem hiding this comment.
This is tricky since that the optimization here is very specific to constraints (e.g., count inversion at a cost of 3 constraints).
| let standard_cost = | ||
| standard_be_bits.len() + standard_be_bits.iter().filter(|x| **x == true).count(); |
There was a problem hiding this comment.
Does it make sense to make a pow_by_const trait, with two impls, but eventually three, naf, standard, and windowed?
Then we can have four methods, pow, pow_r1cs, pow_native_cost, and pow_r1cs_cost?
There was a problem hiding this comment.
Or does that seem like a premature abstraction? If so, I at least recommend splitting out the pow and r1cs_cost into separate functions for code readability & reuse in the future.
There was a problem hiding this comment.
I got the idea. It seems better to have a separate function that simply returns what is the best sequence, and then this function in fp/mod should just execute it.
I agree and will make the change. Let me think about where to put this function since it is naturally r1cs...
(Note: later it might also be used for group elements)
There was a problem hiding this comment.
Let me try to split it and will keep you posted.
Description
Warning: this PR is more complicated than one may expect.
This PR changes
FpVar's implementation ofpow_by_constantto a special version that uses NAF.However, there are several subtle issues.
First, for correctness and soundness,
base= 0,exp!= 0, the result should be zero, and the constraint system should be satisfied. This means that one cannot simply useinverseto compute the inverse of the base, as it may create an unsatisfiable constraint.base= 0,exp= 0, there should be panic, and the constraint system is guaranteed to be unsatisfied.Second, for efficiency,
closes: #55
Note that currently
algebra'spowdoes not panic whenbase= 0 andexp= 0, which will be handled in a separate PR.This PR is associated with four tests.
For review, this PR will need some effort, as it appears to be quite complicated.
If you find any good ideas to simplify the code, please propose. The current code is a little bit longer than expected (due to the handling of soundness in respect to the corner cases).
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pendingsection inCHANGELOG.mdFiles changedin the Github PR explorer